Expose record version and txnLogKey as separate audit clocks - #2497
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces dual-clock audit records (harper#2412 stage 0b) to separate a record's own version (version) from its transaction log key (localTime), preventing issues where the two clocks differ during source fills or replication. The feedback identifies a high-severity bug in LMDBTransaction.ts where the transaction argument is omitted in write.commit, causing logTime to incorrectly fall back to the record version and breaking dual-clock separation on LMDB. Additionally, the feedback suggests using strict assertions (assert.strictEqual and assert.deepStrictEqual) in the new test files to prevent type-coercion bugs.
kriszyp
changed the title
…DB audit records On RocksDB an audit record now carries the two roles LMDB has always had: `version` is the record's own version (LWW ordering, @updatedTime, ETag) and `localTime` is that entry's key in the per-origin transaction log. The read surface used to overwrite `version` with the log key, so a consumer could not tell an ordering value from a resume position, and #2409's `recordVersion` alias is absorbed back into `version`. The two clocks hold the same value for every write whose record version is its own commit timestamp, so they only diverge on a source fill (#2065) — which is why confusing them stayed invisible until a cache table replicated. Three consequences carry the change: - A write applied from elsewhere carries its own record version (`TransactionWrite.recordVersion`), read in `save()` only when the transaction is `sourceApply` or `isReplay`. A replication receiver stores the origin's version while committing under the origin's log key; one frame can carry writes at different record versions, so this cannot be per-transaction. - Write identity is explicit as `(nodeId, log key)` in `isAuditEntryWrite`. The tombstone removal in `removeAuditEntry` and the audit pass of the blob orphan sweep both gate on it rather than on a legitimately non-unique version, and both retain rather than delete when identity is unknown. - Crash replay delimits transactions by the log key and replays each write at its stored version. Without the second half, a peer holding a fill at version V under log key L would have it restamped at L after an unclean restart, making a later legitimate write between V and L look stale (#2411). No record-format change, no wire-format change, no change to how LMDB stores anything. `additionalAuditRefs[].version` stays log-key addressable, because every consumer follows it straight into `auditStore.get`. Refs #2412 Refs #2411 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WMMWWeFsqFuKduJHNMP6ZY
kriszyp
force-pushed
the
kris/harper-2412-stage0b-dual-clock
branch
from
September 4, 2026 17:52
eda1ffb to
9211de8
Compare
kriszyp
marked this pull request as ready for review
September 7, 2026 12:50
This was referenced Sep 8, 2026
Merged
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
…'s clock split #2497 split the RocksDB audit record's two clocks: `version` is the record's own version and `txnLogKey` is its key in the per-origin transaction log. The read surface no longer overwrites `version` with the log key, and that PR updated the one corrupt-entry drain test it had to `record.txnLogKey`. The onCorruptFrame tests were written before that landed and still read `record.version`. Their synthetic entries carry only a timestamp and no decodable record version, so after the rebase all thirteen collected `undefined` and failed. Read `txnLogKey`, and rename the collectors from `versions` to `logKeys` — they hold log keys, which is the distinction #2497 exists to make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5vXKjCvTH1zasJ3oYquFQ
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
…version (#2497 regression) `getRecordAtTime` reconstructs a record's image at a past audit position by walking the audit chain backwards from the record's head entry. #2497 changed where that walk starts: from the entry's `localTime` to `resolveAuditPosition(... currentEntry.version ...)`, which returns the record version whenever no `additionalAuditRefs` name a divergent key. The two clocks coincide on RocksDB, so that read is correct there. The LMDB audit store is keyed by a `localTime` minted independently of the record version, and LMDB records carry no refs — so the walk's first lookup addresses no entry, breaks immediately, and every reconstructed image comes back as the live record. `test:unit:lmdb` has been red on `main` since 6d5a463: test MQTT connections and commands subscribe to retained record with patch operations: AssertionError: 'update 3' == 'update 2' A durable MQTT session resuming from an earlier position was replayed the current record once per missed write instead of each write's own image. `resolveAuditPosition` now takes the position its holder already reports — a record entry's `localTime`, an audit entry's `previousVersion` — as the fallback, rather than re-deriving one from the version. On RocksDB `localTime` is the version, so nothing there changes; on LMDB it is the audit key, which is what the walk needs. That makes the rule the one `isAuditEntryWrite` already documents: a write's log key is the position its holder reports, and the refs name a divergent one. Refs #2412 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017S9s2kMDR5BL9CD33PhoUn
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
…e record version, so LMDB partial-record history resolves (harper#2275) On LMDB the audit-store key (localTime) and the record version are separate clocks, and a record carries no additionalAuditRefs. #2497 seeded the walk from the version, so the first audit lookup missed and every historical read returned the live record: a durable MQTT subscriber resuming across two patches received the newest state twice and lost the older one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
Fix LMDB partial-record history resolution broken by #2497 (harper#2275), hold the QA-701 409 probe's window open causally, and wait for MQTT deliveries instead of sampling 200ms
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
Round 3 tightened _writeInvalidate/_writeRelocate from `< 0` to `<= 0` to gate the storedReusedVersion marking it added; round 6 replaced that marking with rocksdb-js's VERSION_NOT_UNIQUE_FLAG and deleted the tracking, but left the tightened comparison behind. Nothing in the PR reads it now, and it short-circuits a resolution the guard already performs: precedesExistingVersion breaks a version tie on node NAME, returning 1 when the updating node wins and -1 when the existing one does, so only a same-node tie reaches 0. Skipping that 0 discards the audit-clock write the dual-clock work (#2497, merged after this branch's tip) records for an applied invalidate or relocate whose log key advanced while its record version did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMmCvW2DSbPVnVeDBQVPTj
kriszyp
added a commit
that referenced
this pull request
Sep 8, 2026
…'s clock split #2497 split the RocksDB audit record's two clocks: `version` is the record's own version and `txnLogKey` is its key in the per-origin transaction log. The read surface no longer overwrites `version` with the log key, and that PR updated the one corrupt-entry drain test it had to `record.txnLogKey`. The onCorruptFrame tests were written before that landed and still read `record.version`. Their synthetic entries carry only a timestamp and no decodable record version, so after the rebase all thirteen collected `undefined` and failed. Read `txnLogKey`, and rename the collectors from `versions` to `logKeys` — they hold log keys, which is the distinction #2497 exists to make. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G5vXKjCvTH1zasJ3oYquFQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RocksDB audit records have two clocks with different jobs:
versionis the record's LWW version, whiletxnLogKeyis the entry's position in the origin node's transaction log.versiondrives ordering,@updatedTime, and ETags;txnLogKeydrives replay, resume, grouping, and write identity.Previously the internal audit API called the second value
localTime, inherited from the deprecated LMDB model. That name is actively misleading once a replicated entry preserves both the origin's record version and the origin's log position. This PR makestxnLogKeythe canonical internal name.localTimeremains only on the existing listener, history, and pro-to-core compatibility payloads; this is not an API migration.Why the clocks must remain separate
For an ordinary local write, both values are the transaction timestamp. They diverge for a source fill: the record keeps the source's
lastModified, while its audit entry is committed under the fill transaction's log key. Usingversionas a cursor moves replication into the record-ordering clock domain; usingtxnLogKeyas the record version can make a legitimate later write look stale.Applied and replayed writes therefore preserve the body
version, but bound the effective write version tomin(version, txnLogKey). The bound is required by the current audit format: an audit-only out-of-order entry can carry the later surviving record version in its body. Without the bound, recovery could resurrect the superseded value at that later version.What changes
{ version, txnLogKey }internally on both storage engines.txnLogKey; each write retains its ownversion.txnLogKeydomain.(nodeId, txnLogKey)identity, including equal record versions with distinct same-origin log keys; crash replay does not treat the audit entry it is replaying as proof that the primary mutation committed.(nodeId, txnLogKey), including the fallback audit walk when an exact keyed lookup misses.localTimefield.This is the core half of Harper Pro #812, "Preserve origin record versions and transaction-log keys during replication". The two PRs ship as one artifact.
Verification
npm run build: clean.The focused suites above were rerun on the exact pushed head. CI remains the broader verification surface.
Scope and follow-ups
This is the naming and receive/replay stage of #2412. It deliberately does not redesign the stored audit entry. A later format stage can replace the additional audit-head reference, remove the overloaded audit-only body version, and eliminate the temporary
min(version, txnLogKey)normalization. Publish messages remain outside sourced-record clock separation: a legitimate locally originated message hasversion === txnLogKeyand does not carry a source-owned record version. Deprecated LMDB keeps its legacy transaction-version apply behavior; it is not the vocabulary source or a constraint on the RocksDB contract.Refs #2412
Refs #2411
Review-Coverage: authored=unknown; ran=none; rounds=1 @ 755329e
Human-Review-Need: 4 @ 755329e